feat: show form item Additional Input Fields in the Purchase Details lines view - #1074
Merged
smarcet merged 3 commits intoSep 17, 2026
Merged
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Comment |
smarcet
self-requested a review
September 17, 2026 21:21
smarcet
pushed a commit
that referenced
this pull request
Sep 17, 2026
…lines view (#1074) * feat: show additional input fields in the purchase details lines view * test: name the canceled-line additional fields test for what it asserts * refactor: trim additional fields comments and drop two framework-level tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref: https://app.clickup.com/t/86bc2ekbd
What this does
Show organizers use the Purchase Details report (Sponsors > Reports > Purchase Details, Lines view) to fulfil sponsor orders on site. The answers sponsors give in a form item's Additional Input Fields (chair color, labor start time, number of people) are part of what has to be delivered, but the Lines view rendered a fixed set of 13 columns and would have silently ignored them.
This adds an Additional Fields column so those answers are visible next to each line on screen and on the printed manifest, not only in the CSV export.
How
Three files, purely additive presentation:
src/components/sponsors/reports/LinesManifestView.js: a module-privateAdditionalFieldscell component, oneHEADERSentry aftercol_notes, one<TableCell>after the notes cell.src/i18n/en.json:sponsor_reports_page.col_additional_fields, wording matched to the backend CSV header so the screen, the print view and the export all name the column the same thing.No reducer or action change was needed, and I confirmed that rather than assuming it:
buildPurchaseLinesQuerysends no sparse-fieldset param and the lines reducer destructuresdatawholesale, so the new field reaches the cell untouched. That was the one thing that could have made this a no-op in production.API contract
Each row of
GET purchase-details/linescarriesadditional_fields, either null or a list of{label, value}. Both are always strings and arrive display-ready: option text already resolved to the option name, DateTime already formatted in UTC, CheckBox already Yes or No. The key can also be absent on older rows.The UI deliberately does not format or translate the values. The API owns the display format so the screen, the print view and the CSV always agree. Field labels are sponsor data, so they are correctly not run through i18n.
Shipped in sponsor-reports-api https://github.com/fntechgit/sponsor-reports-api/pull/43.
Three decisions worth a reviewer's attention
The cell component lives in
LinesManifestView.js, not its own file, and is not exported. The repo's conventions fold a single-consumer helper into the component that owns the concept, and this file is the cited exemplar of that rule. It is private because nothing outside the file consumes it, matching the in-filelineTotalLabelprecedent;Destinationis exported only becauseByItemViewimports it.The React key is index based, with the eslint disable kept. An answer carries no stable id and a form may legitimately define two fields sharing a label, so a label key is not safe. This file already carries the identical disable on its row key for the identical reason. The disable sits inside the tag directly above
key=because the one line form exceeds prettier's printWidth, and after prettier rewraps it the disable no longer applies andeslint --fixhard errors.No width cap on the new cell. The stated consistency target is the Notes cell, which is a bare
<TableCell>with no width or wrap styling, so consistency means adding none. MUI cells already wrap.Tests
Two new tests, plus one existing test updated.
HEADERSversus cells desync, which would silently misalign every column to its right.A deliberate trade on test coverage. Two of the ticket's acceptance criteria, values rendering markup literally and a canceled line's cell being struck through, have no direct unit test. Both are satisfied by not writing something: React escapes by default, and the strikethrough comes from the pre-existing row level
& tdsx, which needed no new code. A test whose subject is framework behavior is what the testing playbook's "What NOT to Test" rules out, so an acceptance criterion does not automatically earn a test here. Raising it explicitly rather than leaving the gap silent.Verification
yarn test src/components/sponsors/reports: 15 suites, 121 tests, all passingyarn linton the changed files: 0 errorsskills/react-frontend.mdsections 1 through 8 and the show-admin playbooks for i18n, testing, list pages and reuse before build: no violationsTwo notes on the wider suite. The full
yarn testrun has one failure, insponsor-purchases-list.test.js, which reproduces on a cleanmastercheckout and is not from this branch. And the ticket's criterion "yarn lint reports 0 errors" is not satisfiable as written:eslint srcreports over a thousand pre-existing errors repo wide, and what CI actually gates islint-stagedon changed files, which are clean. Worth rewording that criterion on the ticket.Still open on the ticket
Two acceptance criteria need a deployed sponsor-reports-api and stay open after this merges:
additional_fieldsPrint is the real risk and it is not unit testable.
TableContainergives horizontal scrolling on screen, but in print the report body is absolutely positioned at full width, so overflow clips rather than scrolls, and this is now column 14. The browser check should look specifically at clipping, not merely at whether the column appears.Please do not close the ticket on this PR alone.
Out of scope